perf(desktop): reduce UI stalls during tool output - #3921
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P1] Batching caches unbounded raw output before redaction/caps
app-shell-session-events.ts:167-178 enqueues full tool_output_delta into pendingEvents; redaction and size caps only happen in applyToolOutputChunk() at frame time — burst can hold large unredacted data pre-paint → OOM risk.
Fix: redact and bound/coalesce at enqueue, not at paint.
[P2] Delayed frame resurrects cleared session state
Frame callback unconditionally projects pending events; cleanup doesn't drop batch and clear runs separately → delete session then frame creates placeholder turn/tool for deleted ID.
Fix: tie drop/cancel to teardown/delete with regression for delayed-frame-after-clear.
Hosted test/windows_recovery: SUCCESS, but code is NO-GO.
简体中文
批量缓存与清理竞争问题。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Follow-up correction in
Verification:
The remaining regressions cover the intended frame publication bound and the delayed-frame-after-clear race. |
e9e0bbc to
e98bbf0
Compare
e98bbf0 to
fcfa981
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head fcfa981eb15d9127e94b486d2e240923d72e77c8. Approving with comments (P2 only).
The delayed-frame-after-clear race is fixed (dropDisplayEvents from clearSessionRendererState). Agreed that a synchronous flush every N events would fight this PR's animation-frame batching, so that path should not come back.
[P2] Putting tool_output_delta through scheduleDisplayEvent still leaves pendingEvents uncapped. Each producer chunk is already bounded, but the renderer list is not: a flood can sit until rAF (or the 100ms timeout) and then apply as one long paint. Please drop-oldest / coalesce in the pending structure (reuse the existing 200-chunk / 16KiB live-turn cap) without extra setState.
CI test on this SHA is green. Not blocking.
Publish high-frequency tool output once per animation frame while preserving synchronous lifecycle ordering.\n\nBound pending output with the existing live-turn limits and discard queued events when a session is cleared.\n\nGenerated-by: Codex
0419549 to
24a025e
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for addressing the remaining queue-bound concern. Pending tool output now reuses the existing per-tool 200 chunks / 16 KiB limits, retains the newest bounded suffix without an extra state publication, and still flushes lifecycle events synchronously. Session teardown also drops queued display events before clearing its projection.
The regression covers both limits, one publication per frame, and the delayed-frame teardown race. I verified exact head 24a025e9066191a987cabf4c572f9ff5d1e638cf; test passes, the previous P2 is resolved, and I found no remaining P0–P3 issues. Approving.
Review analysis was assisted by Codex; Astro-Han verified the queue authority, frame publication, and teardown paths and owns this approval.
中文对照
谢谢解决剩余的队列上限问题。待发布的 tool output 现在复用现有的单 tool 200 chunks / 16 KiB 限制，在不额外发布 state 的情况下保留最新的有界后缀；生命周期事件仍会同步 flush。Session teardown 也会在清理 projection 前丢弃排队中的 display event。
回归测试覆盖了两项上限、每帧只发布一次，以及延迟 frame 与 teardown 的竞态。我验证了精确 head 24a025e9066191a987cabf4c572f9ff5d1e638cf；test 通过，之前的 P2 已解决，没有发现剩余的 P0–P3 问题，可以 Approve。
本次审查分析由 Codex 协助；Astro-Han 核验了队列 authority、frame publication 和 teardown 路径，并对本次批准负责。
Batch high-frequency tool output through the existing animation-frame display queue. Reuse the live-turn output limits for pending events and discard queued display work during Session teardown. Generated-by: Codex
2 participants
Summary
Live tool output currently publishes every chunk directly to Renderer state. Frequent chunks trigger repeated React updates within one display frame.
This can make the interface feel unresponsive while the model is working. Expanding tool details can lag, and loading animations can drop frames.
tool_output_deltapublishes a separate state update.All output chunks remain ordered and complete. Lifecycle and readiness events remain synchronous and flush pending output before they run. Session teardown discards its queued events so a delayed frame cannot restore cleared state.
No screenshot or recording is included. This problem affects transient frame pacing during live streaming, which static images cannot show reliably.
Verification
npm --workspace @maka/desktop run build:mainnode --test apps/desktop/dist/main/__tests__/streaming-handoff.test.jsgit diff --checkAI use
Select exactly one:
Tool(s) and scope: Codex traced the Renderer event flow, implemented frame batching and the session teardown fix, added regression tests, and ran the listed checks. The affected commits include
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?